Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiler refactor: extract type_from_dependencies #12437

Merged
merged 2 commits into from
Sep 7, 2022

Conversation

asterite
Copy link
Member

@asterite asterite commented Sep 1, 2022

Two places in the code had a similar logic: computing a node's type from its dependencies. One of the branches actually involved a bit of duplicate logic: checking for the dependencies size first, but then Type.merge(...) also checks for the size again.

This PR moves that logic to a helper method: type_from_dependencies. We could simply call Type.merge(dependencies) instead, but for later refactors I have in mind overriding type_from_dependencies for some specific AST nodes. For example, an if node is currently bound to its then and else nodes, putting them in the dependencies array. But that's not needed: we could override type_from_dependencies for If to return the type union of the then and else node. That way we don't need to store an array of two elements in dependencies for every if in the program.

However, I'd like to do those optimizations in a separate PR to keep things small and easier to review.

@straight-shoota straight-shoota added this to the 1.6.0 milestone Sep 6, 2022
@asterite
Copy link
Member Author

asterite commented Sep 6, 2022

It seems I did a similar refactor in #12405 so if we merge that one we should close this one.

@straight-shoota
Copy link
Member

Yeah, let's merge this one first, then the other. That should make the best commit history.

@asterite
Copy link
Member Author

asterite commented Sep 6, 2022

Sounds good! I actually introduced another commit in the other PR, but I'll revert it with a force-push.

@asterite asterite merged commit f021424 into master Sep 7, 2022
@asterite asterite deleted the refactor/type_from_dependencies branch September 7, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants